Skip to content

[FIX] git: keep the last line of output when pulling worktrees - #177

Merged
brinkflew merged 2 commits into
betafrom
avs-fix-pull-console-output
Aug 6, 2026
Merged

[FIX] git: keep the last line of output when pulling worktrees#177
brinkflew merged 2 commits into
betafrom
avs-fix-pull-console-output

Conversation

@brinkflew

Copy link
Copy Markdown
Contributor

Description

odev pull was swallowing its last line of output: the summary for the final repository of the final worktree was printed and then immediately erased.

FetchCommand.run ends with self.console.clear_line() to drop the blank line that Command.table unconditionally appends after each worktree table — correct for fetch. But PullCommand overrides run_hook with plain logger.info lines and never emitted that trailing blank, so the cleanup ate a real line of output instead.

The fix gives the pull hook the same output shape as the fetch one (option 3 in the issue):

  • each worktree is introduced by the same left-aligned cyan rule fetch uses as its table title, extracted from Console.table into a reusable Console.title_rule;
  • the hook closes with a blank line, so run's cleanup consumes a blank line rather than content;
  • the now-redundant for worktree '<name>' fragment is dropped from the log messages, since the rule carries the worktree name;
  • the contract is documented on FetchCommand.run_hook so future overrides keep it.

Before:

[i] No pending changes for worktree '18.0' in 'odoo/odoo' for version '18.0'
[i] No pending changes for worktree '18.0' in 'odoo/enterprise' for version '18.0'
                                                    <-- design-themes line erased

After:

─ 18.0 ─────────────────────────────────────────────
[i] No pending changes in 'odoo/odoo' for version '18.0'
[i] No pending changes in 'odoo/enterprise' for version '18.0'
[i] No pending changes in 'odoo/design-themes' for version '18.0'

odev fetch and odev worktree --list rendering is unchanged.

Linked Issues

Compliance

  • I have read the contribution guide
  • I made sure the documentation is up-to-date both in doctrings and the docs directory
  • I have added or modified unit tests where necessary
  • I have added new libraries to the requirements.txt file, if any
  • I have incremented the version number according the versioning guide
  • The PR contains my changes only and no other external commit

🤖 Generated with Claude Code

https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du

`FetchCommand.run` clears the blank line that `Command.table` appends after
the last worktree summary. `PullCommand` overrides `run_hook` with plain log
lines and never emitted that trailing blank, so the cleanup erased the summary
of the last repository instead.

Give the pull hook the same output shape as the fetch one: a section title
introducing each worktree and a blank line closing it. The worktree name moves
from every log message to that title, and the contract is now documented on
`FetchCommand.run_hook` so future overrides keep it.

Claude-Session: https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du
sea-odoo
sea-odoo previously approved these changes Jul 30, 2026
@brinkflew
brinkflew merged commit d1a1a32 into beta Aug 6, 2026
@brinkflew
brinkflew deleted the avs-fix-pull-console-output branch August 6, 2026 21:30
brinkflew added a commit that referenced this pull request Aug 6, 2026
odev.common.logging configures logging on import, but logging.basicConfig is a no-op once the
root logger has handlers: whether odev's handler gets installed depends on whether that import
happens before or after pytest sets its own up, and importing the sandbox from conftest tips it.
Combined with the capture added by #176 every record then reached the output twice, and a plain
logger.info became a console.print that #177's run_hook test asserts on. The handler is dropped
in pytest_configure so the suite no longer depends on import order.

Also resolves the version command test, which this branch made namespace-agnostic while #175
rewrote it, and bumps the version to 4.31.2, one increment above the base branch.
brinkflew added a commit that referenced this pull request Aug 27, 2026
…st suite (#178)

## Why

This started as filling coverage gaps, and each step surfaced the next:

1. `.coveragerc` gates at 60% and the suite sat at **64%**, with the gap widest on pure logic the rest of the framework leans on. Writing those tests surfaced **six defects in the helpers** — without fixing them the tests would have pinned broken behaviour.
2. Verifying the fixes meant running the suite repeatedly, which is when it became clear that **two suites cannot run at once**: identical invocations produced anywhere from 0 to 68 failures, and the machine had accumulated 16 orphaned `/tmp/odev-test-*` directories.
3. Isolating the runs revealed that three concurrent suites exhaust PostgreSQL's connection slots, which turned out to be a **connection leak in odev itself**, not in the suite.

The four sections below are independent and the commits are ordered to be reviewed in sequence.

## 1. Helper defects — `42569c4`

| Location | Defect |
|---|---|
| `connectors/postgres.py` `columns_exist` | Returned `[]` when **none** of the requested columns existed — indistinguishable from all being present. `CREATE TABLE IF NOT EXISTS` leaves an existing table alone, so the missing-columns pass is the only thing that can migrate a table created from an older definition; it silently added nothing. |
| `postgres.py` `PostgresDatabase.tables` | A class attribute, so every instance shared one registry and tables from different databases collided on their name alone. |
| `string.py` `quote` | Chose its delimiter with `max()` over both quote offsets, picking the **last** rather than the first, mis-quoting any string mixing them. |
| `version.py` `OdooVersion.__bool__` | Always `True` — `module` is padded to `MIN_VERSION_LENGTH` and is never an empty tuple. |
| `string.py` `min_indent` | Raised `ValueError` on a text without any non-blank line, reachable from `odev help` through `dedent`. |
| `float_to_hours`, `strip_styles` | Broken, but called nowhere in odev nor in the plugins. **Left alone**, documented in the tests with the correction spelled out. |

`columns_exist` has exactly one caller, and it runs after `CREATE TABLE IF NOT EXISTS`, so the fix cannot make it issue `ALTER TABLE` against a missing table.

## 2. Coverage for the untested helpers — `be95197`

- **`test_string.py`** (new) — `string.py` had no test module at all, despite backing `odev help`, `odev history` and the local database listing query. Sizes and their round-trip, indentation, joining, the `dirty_only` × `force_single` quoting matrix, Rich markup helpers, and the `help` column alignment contract.
- **`test_git_worktree.py`** (new) — `connectors/git.py` was the least-covered large module (34%), and its `GitWorktree` parser turns `git worktree list --porcelain` into the objects the whole `fetch` / `pull` / `worktree` family works with. Porcelain parsing (branch, detached, bare, locked, prunable with reasons), the `-odev-` local-branch split that `create_worktree` writes and `fetch` / `pull` read back, identity by path, and `pending_changes` including the two swallowed `GitCommandError` messages. No network, no real repository.
- **`test_postgres_table.py`** (new) — `PostgresTable.__add_missing_column`, the datastore's migration path, was entirely unreached; this covers it including the `InvalidTableDefinition` primary-key branch.
- **`test_version.py`** — ordering (`15.0 < 16.0 < saas-16.4 < 17.0 < master`) is what actually picks a revision at runtime and nothing compared two versions.

Corrections to existing tests, in the same commit:

- **`test_bash.py` shelled out to a real `sudo cat >> /etc/shadow`.** The premise that the command fails only holds for an unprivileged user whose shell cannot open the redirection — a machine granting passwordless sudo runs it for real, and as root it appends to the file or hangs on stdin. The subprocess and the effective user are now simulated, which also lets the elevation path be asserted rather than inferred.
- `test_odev.py` left a command line behind in `sys.argv` for whichever test ran next.
- `tests/fixtures/case.py` — `_patches` was a list defined on `OdevTestCase` and mutated through `cls._patches.append`, so every subclass shared it and each class tore down the patches of all the classes before it.

## 3. An isolated, self-cleaning test suite — `87dc0e4`, `86630d3`, `716c8e1`

`Odev.name` was the constant `"odev-test"` and **every** shared resource derived from it, so two suites shared one namespace and actively destroyed each other:

- `test_99_delete_expression` ran `odev delete --expression "^odev-test-[a-z0-9]{8}" --include-whitelisted` against the real PostgreSQL, deleting a concurrent run's databases.
- `PostgresDatabase.drop()` terminates every backend on `datname`, so each class teardown killed a concurrent run's cursors.
- `CREATE TABLE IF NOT EXISTS` is not atomic, and `Config.save()` truncate-writes a fixed path — hence `UniqueViolation` on `pg_type_typname_nsp_index` and `DuplicateOptionError` from a torn config.

A run now claims a sandbox named after itself and holds an exclusive `flock` on it for its whole life. Everything — datastore, test databases, config, temp directories — is named after it or nested under it. Cleanup runs at `pytest_sessionfinish`, which pytest calls from a `finally`, so `Ctrl+C` is covered; `SIGTERM` becomes the same orderly exit; and the next run's sweep collects whatever a `SIGKILL` left, because the kernel releases the lock when the owner dies whatever the cause.

**The suite was also writing outside its sandbox**, which is worth a look on its own:

- `TestSetup` ran the install scripts against their real destinations, so running the suite **repointed the developer's `~/.local/bin/odev` and bash-completion symlinks at whichever checkout it ran from**. `symlink.py` computed the destination halfway through creating it, leaving no way to redirect it; that decision moves to `link_path`.
- Tests cloned into the real `~/odoo/repositories`. The repositories, dumps and upgrade paths now point inside the sandbox, as does `CONFIG_DIR` — which also means the suite no longer picks up whichever plugins the developer happens to have installed, so a local run and CI exercise the same code.

`87dc0e4` is a separate product fix this surfaced: `LocalDatabase.is_odoo` checks that a database exists and then connects to it, and any process can drop it in between — `odev list` inspects every database in turn and would fail outright because one went away.

Interrupt handling is covered by `tests/tests/common/test_interrupts.py`: odev captures `SIGINT` around every query to cancel just that statement, so a `Ctrl+C` was previously swallowed and the run carried on. Letting it through instead abandons the connection mid-statement, so the interrupt is recorded and acted upon at the next test boundary.

## 4. Connection lifetime — `47499cb`, `cd07007`

Both database context managers built a **second, unconnected** connector to close instead of the one they had connected, so `disconnect()` did nothing and the connection stayed open until the garbage collector got to it:

```python
def __enter__(self):
    self.connector = self._connector_class(self.name).__enter__()   # connector A, connected
    return self

def __exit__(self, *args):
    self._connector_class(self.name).__exit__(*args)                # connector B, never connected
```

`ensure_connected` runs every database method inside its own block and those blocks nest — `is_odoo` opens one and then calls `table_exists`, which opens another — so this meant a fresh backend per call.

Closing the right connector is **not enough on its own**: an inner block would close the connection the enclosing one is still using. The blocks are now reentrant and share a single connector, counted in `PostgresConnectorMixin` so both classes get the same behaviour. The datastore holds its connection instead of reopening it per read — every command reads it and it lives as long as the process, which is not true of the databases odev walks through for `list` or `delete`.

A connection pool keyed per database was considered and set aside: `list --all` and `delete --expression` touch **every** database on the server briefly, so a per-database pool would hold one idle backend per Odoo database until the process ends — the very exhaustion this fixes — unless it also grew a global cap and idle eviction.

Measured over a full suite run:

| | before | after |
|---|---|---|
| peak backends held | 42 | **3** |
| mean backends held | 8.7 | **0.8** |
| suite duration | 54.8s | **33.6s** |
| three concurrent suites | died on `max_connections` | **249 passed each**, peak 7 backends |

The speedup was not the goal — it is what a backend fork plus an authentication round-trip per query costs.

## Coverage

| Module | Before | After |
|---|---|---|
| `common/string.py` | 85% | **100%** |
| `common/version.py` | 96% | **100%** |
| `common/postgres.py` | 81% | **93%** |
| `common/connectors/git.py` | 34% | **40%** |
| **Total** | **64%** | **65%** |

## Verification

- `pytest tests` — **249 passed**, from 242 on the first revision
- Two and three concurrent suites — **249 passed each**, repeatedly, leaving zero directories and zero databases behind
- `SIGINT`, `SIGTERM` and `SIGKILL` mid-run — each verified to leave nothing behind, the last one via the next run's sweep
- `odev list --all`, `odev history`, `odev version` — smoke-checked, no connections surviving the process
- `pre-commit run --all-files` — clean
- `basedpyright` — 3 errors, all pre-existing on `beta`; **0 new**

## Notes for reviewers

- `odev/_version.py` is bumped once, to `4.29.10`. `origin/beta` is at `4.29.9`; PRs #175, #176 and #177 each bump from the same base, so whichever merges second needs a one-line rebase.
- `LocalDatabase.connector: PostgresConnector | None = None` was removed as dead — `ConnectorMixin.__init__` overwrites it with the connector *class* at construction, which also meant the `if self.connector is not None` guard in `_restore` never protected anything. It is now the `isinstance` check `drop()` already used.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants